binary-search: add json test data #349
Conversation
| }, | ||
| { | ||
| "description": "finds a value in the middle of an array of odd length", | ||
| "array": [1, 3, 5, 8, 13, 34, 34, 55, 89, 144, 233, 377, 634], |
There was a problem hiding this comment.
Should there be 2 34's here?
There was a problem hiding this comment.
Yes, this test will fail if someone just starts at the beginning of the array and returns the index on the first occurrence of the value that should be searched.
There was a problem hiding this comment.
That sounds like it should be a separate test case.
- 'find value in the middle of an array of odd length'
- 'find correct value for duplicated element'
There was a problem hiding this comment.
Also you probably need test cases for when the 'correct' duplicated element is the first element as well so you can't fake it by reversing the list or taking the highest index or some other similar workaround.
There was a problem hiding this comment.
(I suggest that none of the test arrays should have duplicate elements.)
There was a problem hiding this comment.
Sorry, I am a bit confused here @Insti . First you explained how to add more test cases that check the binary search is done somewhat correctly, then you said in brackets (?) that we should not do this.
Maybe you can clarify and explain a bit why you would not add these kind of tests.
My thinking was that since most languages don't easily allow counting the number of times the array is accessed this would be an easy chance to avoid that some implementations that do no binary search at all pass all the test.
You are definitely right on the separate test case(s) though.
There was a problem hiding this comment.
If there are duplicate elements, the minimal case is: [ 1, 1 ] Is the correct answer 0 or 1?
(If 0, why is rounding up wrong and rounding down right?)
Testing implementation details can be tricky.
IF you do want to test implementation, then test it directly and assert that the algorithm is being implemented correctly by doing something like adding a 'depth' parameter, or asking for the array result after a certain number of iterations.
The simplest thing to do is make sure the test arrays are sorted and only contain unique values, and rely on the reviewers to call out incorrect algorithms. This is the method used in many other exercises. (And would be my preferred method.)
There was a problem hiding this comment.
Thanks for taking the time to explain. I nobody else votes in favour of keeping it, I will change it to use only unique values. I might not have a chance to update the PR before the weekend. But maybe other points will come up and I can change it all together next week.
There was a problem hiding this comment.
That's a really great observation, @Insti - I second the change to use only unique values.
There was a problem hiding this comment.
PR is updated to use only unique values
|
There probably should be a test for an even length array. |
|
Unfortunately the test sets in the different tracks were very divers for this problem and I couldn't find a single one that was just right to use it directly for the json file. So I build up the content with inspiration/parts from different tracks. |
|
No problem @junedev, all your work on this is much appreciated! |
|
We good with these? I think the only potential additions I would have are
These seem like separate cases than the existing, so could be useful. If you agree, add them and I think that will be that. If you think they aren't (and can explain to me) I think I'd take what's here as is. |
|
@petertseng I like the additional test cases. I will update the PR accordingly. - Done. |
|
@petertseng Is this ready to be merged? |
|
I say yes! |
|
Merged! 🎉 |
Add a Gitter chat badge to README.md
Following the instructions proposed by @IanWhitney in #230, I created the missing json file with the test data for binary-search. Once this is merged https://github.com/exercism/todo/issues/73 can be closed.
There are two open issues regarding the exercise:
#234 - Following (and agreeing with) the majority there I did not include checking for sorted input. I also included a respective comment in the json file. The issue can be closed once the new test data is propagated through the tracks.
#244 - I included a comment with the summary of what was discussed there but the issue seems only partly addressed by this as there are additional ideas on how/where to address the issue.